Code & Security Review

Critical Issues

None.

Summary

The fix correctly addresses the main-actor beachball (#404) with two well-scoped changes:

1. shell() truly yields — replacing waitUntilExit() with withCheckedThrowingContinuation + terminationHandler, and marking it nonisolated. shell() uses no instance state, so dropping main-actor isolation is safe; all existing callers (worktree list, remote get-url, gh issue view, gh pr view, repo clone) still await it correctly.
2. prepareReviewClone off-main — the gh/git/file-prep block moves into a nonisolated static helper run via Task.detached(.userInitiated), leaving only cheap state mutations on the main actor. env is captured as ShellEnvironment (public final class … : Sendable), and Scaffolder.bundled* / buildReviewPrompt are plain non-isolated statics, so the off-main hop is concurrency-clean.

Security Review

Strengths:

• No new external input surface; PR URL parsing and command args are unchanged from the prior implementation. No shell string interpolation — args are passed as an array to /usr/bin/env, so no command-injection vector is introduced.
• Error-message construction now prefers stderr and falls back to stdout; neither is surfaced to users beyond NSLog, consistent with prior behavior.

Concerns: None.

Code Quality

• Continuation safety is correct. terminationHandler is installed before process.run(), so the success path resumes exactly once. The catch on launch failure defensively clears terminationHandler = nil before resuming, preventing a double-resume — a nice touch that matches the documented macOS launch-failure edge case.
• Behavioral parity preserved. PR-metadata fetch failure aborts kickoff (throw → caught → return nil + NSLog), matching the old guard; git fetch/checkout/pull and clone errors remain tolerated via try?. Kickoff serialization in AppDelegate.enqueueReviewKickoff is untouched, so PR ordering is preserved.

Findings

Green — pipe read-after-exit comment is misleading (Sources/Crow/App/SessionService.swift:830-833, 845-853). The doc comment claims reads-after-exit means "no deadlock from a full pipe blocking the child." That reasoning is backwards: draining readDataToEndOfFile only after terminationHandler fires is precisely the pattern that can deadlock if a child writes more than the ~64KB pipe buffer to stdout/stderr and then blocks on write(), never exiting — so the handler never fires and the continuation never resumes. This is not a regression (the old waitUntilExit() path had the identical property) and is harmless for the current callers, whose output is small (gh/git to a non-tty emit minimal progress). But since runShellAsync is now a reusable static helper, consider either correcting the comment or draining the pipes concurrently (e.g. readabilityHandler/background reads) before reusing it for high-volume commands like gh pr diff on a large PR.

Green — no direct unit coverage for runShellAsync. Subprocess runners are awkward to unit-test, and the existing 145-test suite exercises the surrounding paths, so this is acceptable; a small test against a trivial /usr/bin/env true/false would still be cheap insurance for the exactly-once-resume contract.

Note: I could not build locally — swift build fails on a missing Frameworks/GhosttyKit.xcframework binary artifact, which is an environmental/checkout issue unrelated to this diff. The change is self-contained to one file; the PR reports a clean swift build --arch arm64 and 145 passing tests.

Summary Table

Recommendation: Approve — driven by [0 Red, 0 Yellow, 2 Green] findings. Correct, concurrency-clean fix that resolves the beachball without altering kickoff ordering or behavior on error paths.

🐦‍⬛ Reviewed by Crow via Claude Code